Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: gentool config file ignored bug #941

Closed

Conversation

yama-6
Copy link
Contributor

@yama-6 yama-6 commented Aug 3, 2023

  • Do only one thing
  • Non breaking API changes
  • Tested

What did this pull request do?

Before the fix

Part of the content of the gentool configuration file is ignored. This is because the default values set in the command line arguments take priority over them.

Example
$ cd './tools/gentool'
  1. Update gen.yml. Set the value of db to postgres.

$ go run . -c "./gen.yml"

If you could output the logs you would see that, the value of db is executed as mysql. This is because the default value of the command-line arguments is given priority.

After the fix

The content of the gentool configuration file will be reflected. However, if command-line arguments are specified, they will be given priority. This behavior is the same as before the fix.

@yama-6
Copy link
Contributor Author

yama-6 commented Aug 4, 2023

Hi @qqxhb

You made a fix in #942 that relates to this PR.
I think your fix will make gentool give top priority to the configuration file. However, README has the following statement.

command line is the highest priority

I think you need to fix either the README or the implementation.

Sorry for my bad English.

@qqxhb
Copy link
Member

qqxhb commented Aug 4, 2023

@yama-6 没关系,我们可以中文交流。
这个应该是历史实现问题,实际使用场景,我们应该是用yaml配置了就没理由再用命令行去指定,这个没啥意义;整体逻辑看起来也比较乱

@yama-6
Copy link
Contributor Author

yama-6 commented Aug 4, 2023

@qqxhb I understood.

@yama-6 yama-6 closed this Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants